Update tunnel manager to not return error for configuration loading #5188
Conversation
IOS-297 Failed tunnel manager init should show problem report view
Function getInitTunnelManagerOperation() throws a fatal error if tunnel manager cannot load config. We should show the problem report view instead. The code in question: |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/AppDelegate.swift line 439 at r1 (raw file):
AsyncBlockOperation(dispatchQueue: .main) { finish in self.tunnelManager.loadConfiguration { error in if let error {
I don't think we should be dismissing the error and completely ignore it. This means we will continue operation as if nothing happened, but we will most likely be running in an invalid state, which probably means nothing will work as expected.
I suggest to log the error at least so future ourselves can have a clue about what's happening.
The best would be to trigger a way to have loadConfiguration return an error in a real use scenario so we can understand which cases error out, and act accordingly.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)
ios/MullvadVPN/AppDelegate.swift line 439 at r1 (raw file):
Previously, buggmagnet wrote…
I don't think we should be dismissing the error and completely ignore it. This means we will continue operation as if nothing happened, but we will most likely be running in an invalid state, which probably means nothing will work as expected.
I suggest to log the error at least so future ourselves can have a clue about what's happening.
The best would be to trigger a way to have
loadConfigurationreturn an error in a real use scenario so we can understand which cases error out, and act accordingly.
The thing is that this error is misleading to being with. We always treat the operation as successful here and handle state changes elsewhere. According to @pronebird, "worst case if we can’t read settings or device state, we simply pretend they are not there and leave user in logged out state". Also, VPN config will be removed, meaning you'll get another try and system settings prompt when trying to connect later again.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)
ios/MullvadVPN/AppDelegate.swift line 439 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
The thing is that this error is misleading to being with. We always treat the operation as successful here and handle state changes elsewhere. According to @pronebird, "worst case if we can’t read settings or device state, we simply pretend they are not there and leave user in logged out state". Also, VPN config will be removed, meaning you'll get another try and system settings prompt when trying to connect later again.
All right fair enough, would you mind adding a summary of this as a comment in here and we can merge this ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)
ios/MullvadVPN/AppDelegate.swift line 439 at r1 (raw file):
Previously, buggmagnet wrote…
All right fair enough, would you mind adding a summary of this as a comment in here and we can merge this ?
Sure thing. 👍
rablador
force-pushed
the
failed-tunnel-manager-init-should-show-problem-report-view-ios-297
branch
from
25a9e6a to
5b9f9d9
Compare
buggmagnet
force-pushed
the
failed-tunnel-manager-init-should-show-problem-report-view-ios-297
branch
from
5b9f9d9 to
c10006e
Compare
buggmagnet
deleted the
failed-tunnel-manager-init-should-show-problem-report-view-ios-297
branch
It seems we have sufficient handling of cases where we cannot load the config we need. The current path does not allow the load config operation to return a failure/error, so we cannot end up with a thrown fatalError unless there's an implementation error.
This change is